-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use rkyv for JIT Engine #2250
Use rkyv for JIT Engine #2250
Conversation
Since we are going to switch serialization methods, the new one doesn’t have a slowdown if we read the data directly. Thus, we can always operate with processed frame infos
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
lib/engine-jit/src/artifact.rs
Outdated
Ok(serialized) | ||
} | ||
} | ||
|
||
pub fn append_data(mut prev_data: Vec<u8>, data: &[u8], align: u64) -> (Vec<u8>, u64) { | ||
let align = align as usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not much reason to use u64
over usize
in this function, if we're talking about offsets into an array, usize
is the most natural type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u64
will shield us in 32
bit systems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's referring to an offset in memory it doesn't matter. On a 32bit system you only have 32 bits of addressable memory. A usize
is defined to be the size big enough to cover all addressable memory, so when dealing with offsets it's a natural size. It makes sense to cast it to a u64 before writing it if you want it to take up a constant 8 bytes on all platforms, but when dealing with pointers / offsets, a usize
will always be good enough and do the right thing on all platforms.
The blocking bug has been fixed as of 0.6.1. rkyv issue: rkyv/rkyv#116 |
Thanks for the super quick fix @djkoloski! |
bors try |
tryBuild failed: |
This reverts commit 79027cb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good! I've a question about the required vs. optinal feature enable-rkyv
. How do you want to deal with that? serde
has always been optional, so I would expect rkyv
to be optional too. Inside wasmer-cli
, we can force rkyv
though, as it provides much better performance.
Engines serialization mechanisms should be fixed. If you compile with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few minor things, but glad things are working
lib/engine-jit/src/artifact.rs
Outdated
Ok(serialized) | ||
} | ||
} | ||
|
||
pub fn append_data(mut prev_data: Vec<u8>, data: &[u8], align: u64) -> (Vec<u8>, u64) { | ||
let align = align as usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's referring to an offset in memory it doesn't matter. On a 32bit system you only have 32 bits of addressable memory. A usize
is defined to be the size big enough to cover all addressable memory, so when dealing with offsets it's a natural size. It makes sense to cast it to a u64 before writing it if you want it to take up a constant 8 bytes on all platforms, but when dealing with pointers / offsets, a usize
will always be good enough and do the right thing on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, I just noticed 2 quick issues
bors r+ |
Since we are going to switch serialization methods, the new one doesn’t have a slowdown if we read the data directly.
Thus, we can always operate with processed frame infos rather than creating a lazy structure that will be deserialized when needed.
Try it:
git clone https://github.com/wasmerio/wasmer.git -b universal-compiled-info cd wasmer make build-wasmer ./target/release/wasmer run lib/c-api/tests/assets/qjs.wasm --disable-cache -- -h